Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip Static test #516

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Conversation

bpickard22
Copy link
Contributor

In order to run upstream tests in a container with make test we need to be able to skip the static check

@manuelbuil
Copy link
Collaborator

What wrong with staticcheck when run in a container?

Makefile Outdated
@@ -23,7 +25,10 @@ install-tools:
hack/install-kubebuilder-tools.sh

test: build install-tools
hack/test-go.sh
hack/test-go.sh --skip-static-check
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, isn't this going to skip the static check but, we actually want to run it here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no since the default is false

Copy link
Contributor Author

@bpickard22 bpickard22 Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on my local the logic behaves as expected, if you dont pass in skip-static-check arg it will error out, the way its written currently its not an optional arg

hack/test-go.sh Outdated
${BASEDIR}/bin/staticcheck --tags=test ./...
if [ $SKIP_STATIC_CHECK ]
then
echo "SKipped golang staticcheck"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Skipped (not SKipped)

@bpickard22
Copy link
Contributor Author

What wrong with staticcheck when run in a container?

If we want to call make test in a container running in a cluster we cannot assume that static-check will be installed, and you cant run the go install command in -vendor mode

This is currently an issue that we are hitting in our (RedHat) downstream testing

@bpickard22 bpickard22 force-pushed the makefile-updates branch 2 times, most recently from 2a7f911 to 6b8ffe7 Compare November 26, 2024 19:05
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! looking good

@dougbtv
Copy link
Member

dougbtv commented Nov 26, 2024

What wrong with staticcheck when run in a container?

I think Ben's description is incomplete, I think he needs a make target where he can skip this because he's trying to test it a disconnected environment where he can't pull that gomodule, so he needs a target where he can skip it

echo "define argument -s (skip static check)"
exit 1
esac
done
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SKIP_STATIC_CHECK=true

# Parse arguments
while [[ $# -gt 0 ]]; do
  case "$1" in
    -s|--skip-static-check)
      SKIP_STATIC_CHECK=false
      shift
      ;;
    *)
      echo "Invalid argument: $1"
      echo "Usage: $0 [-s|--skip-static-check]"
      exit 1
      ;;
  esac
done

And then you can use it just like ./test.sh --flag-here and not set true/false

@coveralls
Copy link

coveralls commented Nov 26, 2024

Pull Request Test Coverage Report for Build 12039134565

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 38.639%

Totals Coverage Status
Change from base Build 11912515116: 0.0%
Covered Lines: 1971
Relevant Lines: 5101

💛 - Coveralls

In order to run upstream tests in a container with make test we need to
be able to skip the static check

Signed-off-by: Benjamin Pickard <[email protected]>
@dougbtv
Copy link
Member

dougbtv commented Nov 27, 2024

Thank you!

@dougbtv dougbtv merged commit 526edf9 into k8snetworkplumbingwg:master Nov 27, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants